Skip to content

Corporeal StatusEffects#43706

Open
Pok27 wants to merge 6 commits intospace-wizards:masterfrom
Pok27:CorporealStatusEffects
Open

Corporeal StatusEffects#43706
Pok27 wants to merge 6 commits intospace-wizards:masterfrom
Pok27:CorporealStatusEffects

Conversation

@Pok27
Copy link
Copy Markdown
Contributor

@Pok27 Pok27 commented Apr 24, 2026 •
edited
Loading

Requires space-wizards/RobustToolbox#6539

About the PR

Converts Corporeal into new status effects.

Why / Balance

#38575

Technical details

Corporeal visibility is now handled through a refresh-based status effect flow instead of directly changing VisibilityComponent on apply/remove. I added VisibilityModifierStatusComponent with addVisibility and removeVisibility fields so status effects define their visibility changes declaratively.

VisibilityModifierStatusSystem refreshes visibility by relaying RefreshVisibilityModifiersEvent to all active status effects and recomputing the final mask from their combined modifiers. I also added VisibilityModifierStatusTrackerComponent to preserve the baseline visibility between refreshes, and RevenantSystem.MakeVisible() now resets that baseline after manual visibility changes. StatusEffectCorporeal uses the new component.

Media

Requirements

Breaking changes

The Corporeal status effect has been removed and replaced with StatusEffectCorporeal for the new status effect system.
The CorporealComponent has been removed and replaced with CorporealStatusEffectComponent

Changelog

@PJBot PJBot added S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. size/S Denotes a PR that changes 10-99 lines. labels Apr 24, 2026
@Pok27 Pok27 added P3: Standard Priority: Default priority for repository items. T: Cleanup Type: Code clean-up, without being a full refactor or feature D3: Low Difficulty: Some codebase knowledge required. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Apr 24, 2026
iaada
iaada requested changes Apr 24, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@iaada iaada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revenant system is a mess but we don't need to fix it all at once. Fixing up corporeal is a good step forward.

Comment thread Content.Server/Revenant/EntitySystems/RevenantSystem.cs
Comment thread Content.Shared/Revenant/Components/CorporealStatusEffectComponent.cs Outdated
Comment thread Content.Shared/Revenant/EntitySystems/SharedCorporealSystem.cs Outdated
Comment thread Content.Shared/Revenant/EntitySystems/SharedCorporealSystem.cs
public virtual void OnApplied(Entity<CorporealStatusEffectComponent> ent, ref StatusEffectAppliedEvent args)
{
_appearance.SetData(uid, RevenantVisuals.Corporeal, true);
_appearance.SetData(args.Target, RevenantVisuals.Corporeal, true);
Copy link
Copy Markdown
Member

@iaada iaada Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel the same way about the appearance data as I do the collision groups. I'd love to find a more generic solution but I'm drawing a blank.

Copy link
Copy Markdown
Contributor Author

@Pok27 Pok27 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to leave it until the revenant system is refactored

@PJBot PJBot added S: Awaiting Changes Status: Changes are required before another review can happen and removed S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. labels Apr 24, 2026
TheShuEd
TheShuEd reviewed Apr 24, 2026
View reviewed changes
Comment thread Content.Server/Revenant/EntitySystems/CorporealSystem.cs Outdated
@PJBot PJBot added size/M Denotes a PR that changes 100-999 lines. and removed size/S Denotes a PR that changes 10-99 lines. labels Apr 24, 2026
@github-actions github-actions Bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Apr 24, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 24, 2026

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions Bot removed the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Apr 24, 2026
@Pok27 Pok27 requested a review from TheShuEd April 24, 2026 21:19
@PJBot PJBot added S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. and removed S: Awaiting Changes Status: Changes are required before another review can happen labels Apr 24, 2026
iaada
iaada reviewed Apr 25, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@iaada iaada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These status effects are running into an issue I've failed to solve of setter functions conflicting with the expectation of having multiple status effects active. Status effects should be able to operate independently, but the setters require us to remember the original value in order for the entity to be reset.

My instinct is that the proper way to get around this is to use events to query for changes to visibility/collision, and to change these values only through subscriptions and raising the event through some RefreshValues() method. But that's a tall order given them being in engine, especially for physics.

I don't have a good answer. I feel like something will have to bend to preempt apparent bugs or major soaping.

Comment thread Content.Server/Eye/EntitySystems/VisibilityModifierStatusSystem.cs Outdated
Comment on lines +64 to +65
var ev = new RefreshVisibilityModifiersEvent();
RaiseLocalEvent(ent, ref ev);
Copy link
Copy Markdown
Member

@iaada iaada Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this instance I'm not sure the value of an event over StatusEffectsSystem.EnumerateStatusEffects<T>(). The event doesn't seem likely to be used beyond the one subscription.

I think the ideal version might be an event in VisibilitySystem.RefreshVisibility() to get visibility changes instead of a setter API, but I need to think on that. I'm not sure there's an easy solution given that collision groups have a similar problem of using setters and conflicting with potential second or third statuses.

Copy link
Copy Markdown
Contributor Author

@Pok27 Pok27 Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to make a PR in the engine for VisibilitySystem later

Comment thread Content.Server/Eye/EntitySystems/VisibilityModifierStatusSystem.cs Outdated
Comment thread Content.Shared/Revenant/EntitySystems/SharedCorporealSystem.cs Outdated
@Pok27 Pok27 mentioned this pull request Apr 25, 2026
Open
@Pok27 Pok27 added the S: Requires Engine PR Status: Requires a change to Robust Toolbox, for which there is no open PR currently. label Apr 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iaada iaada iaada requested changes

@TheShuEd TheShuEd Awaiting requested review from TheShuEd

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

D3: Low Difficulty: Some codebase knowledge required. P3: Standard Priority: Default priority for repository items. S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. S: Requires Engine PR Status: Requires a change to Robust Toolbox, for which there is no open PR currently. size/M Denotes a PR that changes 100-999 lines. T: Cleanup Type: Code clean-up, without being a full refactor or feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Pok27 @TheShuEd @iaada @PJBot